Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Pagination): add disabled flag for whole component #2586

Merged
merged 5 commits into from Aug 8, 2019

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Jul 24, 2019

What: Adds 'isDisabled' flag to disable the whole pagination component.

See issue: #2315

@patternfly-build
Copy link
Contributor

PatternFly-React preview: https://patternfly-react-pr-2586.surge.sh

@tlabaj tlabaj added css review PF4 React issues for the PF4 core effort enhancement 🚀 labels Jul 24, 2019
Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. The options menu toggle isn't aligning correctly, looks like its only in the pagination component:

Screen Shot 2019-07-25 at 1 20 44 PM

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 25, 2019

The options menu toggle isn't aligning correctly

Am I missing a class?

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Can you update the demo app and the integration test please

karelhala
karelhala previously approved these changes Aug 6, 2019
mcoker
mcoker previously approved these changes Aug 7, 2019
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tlabaj
tlabaj previously approved these changes Aug 7, 2019
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

parentRef = null,
toggleTemplate: ToggleTemplate = '',
}:OptionsToggleProps ) => {
return (
<div className={css(styles.optionsMenuToggle, getModifier(styles, 'plain'), getModifier(styles, 'text'))} >
<div className={css(styles.optionsMenuToggle, isDisabled && getModifier(styles, 'disabled'), getModifier(styles, 'plain'), getModifier(styles, 'text'))} >
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it generally makes more sense to use getModifier if a variable prop can be matched to the style object. In this case i would change these to styles.disabled, styles.plain, etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I thought that was just how we retrieved modifier styles. I can change it to styles.modifiers.disabled and so on.

import React from 'react';
import { Pagination, PaginationVariant } from '@patternfly/react-core';

class PaginationTop extends React.Component {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: class name should be PaginationDisabled. Doesn't affect the rendering but shows up in the sample code

@kmcfaul kmcfaul dismissed stale reviews from tlabaj and mcoker via 0e217f3 August 8, 2019 15:24
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jschuler jschuler merged commit 7683f68 into patternfly:master Aug 8, 2019
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-console@1.12.0
  • @patternfly/react-core@3.84.0
  • @patternfly/react-docs@4.9.26
  • @patternfly/react-inline-edit-extension@2.9.71
  • demo-app-ts@2.17.0
  • @patternfly/react-integration@2.17.0
  • @patternfly/react-table@2.16.10
  • @patternfly/react-topology@2.7.19
  • @patternfly/react-virtualized-extension@1.1.105

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css review enhancement 🚀 PF4 React issues for the PF4 core effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants